-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor: separate insertions and updates in rel table local storage #2982
Conversation
dac8b8d
to
26bfbe3
Compare
d2bd0b0
to
ca5d82a
Compare
26bfbe3
to
161e759
Compare
161e759
to
30ced06
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2982 +/- ##
==========================================
+ Coverage 93.27% 93.36% +0.08%
==========================================
Files 1133 1133
Lines 43123 43060 -63
==========================================
- Hits 40222 40201 -21
+ Misses 2901 2859 -42 ☔ View full report in Codecov by Sentry. |
2fa3389
to
b12ab78
Compare
b12ab78
to
189e842
Compare
189e842
to
34022e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might take a look at this again if I have time before it's merged as I skimmed over a lot of it, but generally it seems fine.
const offset_set_t& deleteInfo) override { | ||
// Avoid unused parameter warnings. | ||
(void)updateInfo; | ||
(void)deleteInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can comment out the parameter names instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still want to do assertions over them. that's why I kept these parameters, but they keep throwing warnings during release build. Might be better ways to handle these?
34022e9
to
fa6d52c
Compare
fa6d52c
to
1f88b3f
Compare
In the middle of reworking local storage. This PR is mainly separating the storage of insertions and updates in local storage.
Note that the following PR will replace the use of DataChunk with a vector of ColumnChunks.
This PR is becoming a bit messy as I did some other refactoring alongside:
unique_ptr
use ofLogicalType
andDataChunk
.NOTE: This PR breaks storage compatibility as it changes the layout of statistics.